Skip to content

fix(last9-cloudwatch): name the averaging window for MSK throughput - #18

Merged
prathamesh-sonpatki merged 2 commits into
masterfrom
prathamesh/eng-1891-window-mean-wording
Sep 10, 2026
Merged

prathamesh-sonpatki merged 2 commits into
masterfrom
prathamesh/eng-1891-window-mean-wording

Conversation

@prathamesh-sonpatki

@prathamesh-sonpatki prathamesh-sonpatki commented Sep 10, 2026 •

Copy link
Copy Markdown
Member

Why

The confirming eval run after #17 merged (34454314035, pinned to 3b39308) had one candidate semantic failure: msk-broker-ingress reported 200 bytes/second where the task asked for the mean over the interval and the answer is 250.

The candidate issued two bare instant queries with no range selector, received only the latest period, and divided that pair (400 / 2) instead of totalling the disjoint periods (2000 / 8). The grader failed it on value, evidence, window_coverage, and raw_observation, which is the correct verdict.

Not a regression from the merge: the eval manifest shows the candidate skill hashes, evaluator hashes, and fixture hash are byte-identical between ca085f7 and 3b39308. Same inputs, different sampling. That case passed in 6 of 7 candidate executions.

The gap

SKILL.md already carries the rule in its statistics table: "Window average = total Sum / total SampleCount. Averaging period averages is wrong when counts differ." Four family references restate the choice at the point of use:

Reference Names latest vs window?
rds-aurora.md yes, "latest companion pair for requested period averages; the weighted template only for a requested window average"
dms.md yes, "For a window mean, use total Sum / total SampleCount per metric"
ec2.md yes, "A window mean from stream summaries needs matched Sum/SampleCount coverage"
elasticache.md yes, "For the latest period, use that matched pair. For a window average, use total Sum / total SampleCount"
msk.md no, only "Use the requested level or average"

MSK was the single family reference with a window-mean scenario that left the window unnamed. This aligns it with the other four rather than adding a new rule.

Change

One sentence in the throughput bullet of references/msk.md: name both cases, and say to read the raw companion series over the window rather than a single instant query returning only the last period. The rate() warning and the broker/topic overlap warning are unchanged.

scripts/check-skill-pack.sh and the selftest pass.

Verification

Nine candidate samples of the failing case against master's skill and nine against this branch: both 9/9 pass, both used a range selector in 9/9, both answered 250. The failure did not reproduce in the pre-fix arm, so this change is unproven and is offered as a documentation clarification, not a demonstrated fix. Details in the comments.

Refs ENG-1891.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

prathamesh-sonpatki and others added 2 commits September 10, 2026 12:21
Confirming eval run 34454314035 caught a candidate reporting 200 instead
of 250 bytes/second for a requested interval mean: it issued bare instant
queries, saw only the latest period, and divided that pair. The grader
failed it on value, evidence, window coverage, and raw observation.

SKILL.md carries the weighted-window rule and the RDS, DMS, EC2, and
ElastiCache references each name the latest-period versus whole-window
distinction. The MSK throughput bullet said only "use the requested level
or average", so it was the one family reference that left the choice open.
Name both cases and say to read the raw companion series over the window
rather than a single instant query.

Refs ENG-1891.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The MSK failure was not only an MSK wording gap. SKILL.md gives the
weighted-window formula in its statistics table, and gives the range
selector separately, in a paragraph about inspecting raw timestamps.
Nothing said that a selector carrying no range returns only the latest
period, so a model could follow the formula correctly and still feed it a
single period, which is what the failing run did.

State it once in the tool reference, where query shape is already
discussed, and give a self-check: the returned sample count must match the
periods the interval should contain. This covers every family rather than
the one whose reference text happened to be thinnest.

Refs ENG-1891.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@prathamesh-sonpatki

Copy link
Copy Markdown
Member Author

Verification result: the fix is unproven. Reporting it straight.

I tested the hypothesis rather than assuming it. The runner accepts --case, so instead of hour-long twelve-case CI runs I ran the exact failing case directly, nine candidate samples against master's skill and nine against this branch, and compared the mechanism, not just the pass count.

Arm Samples Pass Used a range selector Reported value
Pre-fix, master 3b39308 9 9 9/9 250 in all nine
This branch 6a5ec42 9 9 9/9 250 in all nine

The failure did not reproduce once in nine pre-fix attempts. Both arms are at ceiling, so this branch cannot be shown to change anything. My stated mechanism, a bare instant query with no range selector, did occur in the one CI failure, but it is not the model's normal behaviour even without the fix.

So the honest position on the two commits:

  • They state things that are true and were previously unstated. 3332f9c brings msk.md in line with the four sibling references that already name the latest-period versus whole-window choice. 6a5ec42 states what a selector with no range returns, which appeared nowhere despite the formula and the range selector both being documented in separate sections.
  • They cannot be credited with fixing the observed failure. Rate is too low to measure at this sample size. Separating roughly 1-in-7 from zero needs on the order of forty-plus samples per arm, which is one to two hours of model time for a cosmetic-confidence gain.
  • The real safety net is the grader, which caught the bad answer on value, evidence, window_coverage, and raw_observation. That is working as designed and is unchanged by this PR.

I have corrected the "Verification" section of the description, which promised evidence this run did not deliver.

Recommendation: merge as a documentation clarification with no efficacy claim, or close it if you would rather not carry unverifiable changes. I would merge, because both statements are independently correct and one removes a genuine inconsistency between family references, but I do not want the merge justified by a fix that the data does not support.

Broader regression check (run 34465699752, twelve cases, repeats=3) is still running against 3332f9c; I will post it when it lands.

@prathamesh-sonpatki
prathamesh-sonpatki merged commit 6b4638d into master Sep 10, 2026
5 checks passed
@prathamesh-sonpatki
prathamesh-sonpatki deleted the prathamesh/eng-1891-window-mean-wording branch September 10, 2026 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant